❮

predefined role(s) for VACUUM and ANALYZE

  • Jump to comment-1
    nathandbossart@gmail.com2022-07-22T20:37:35+00:00
    Hi hackers, The previous attempt to add a predefined role for VACUUM and ANALYZE [0] resulted in the new pg_checkpoint role in v15. I'd like to try again to add a new role (or multiple new roles) for VACUUM and ANALYZE. The primary motivation for this is to continue chipping away at things that require special privileges or even superuser. VACUUM and ANALYZE typically require table ownership, database ownership, or superuser. And only superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these operations would allow delegating such tasks (e.g., a nightly VACUUM scheduled with pg_cron) to a role with fewer privileges. The attached patch adds a pg_vacuum_analyze role that allows VACUUM and ANALYZE commands on all relations. I started by trying to introduce separate pg_vacuum and pg_analyze roles, but that quickly became complicated because the VACUUM and ANALYZE code is intertwined. To initiate the discussion, here's the simplest thing I could think of. An alternate approach might be to allow using GRANT to manage these privileges, as suggested in the previous thread [1]. Thoughts? [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com [1] https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
    • Jump to comment-1
      bharath.rupireddyforpostgres@gmail.com2022-07-25T07:28:36+00:00
      On Sat, Jul 23, 2022 at 2:07 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > Hi hackers, > > The previous attempt to add a predefined role for VACUUM and ANALYZE [0] > resulted in the new pg_checkpoint role in v15. I'd like to try again to > add a new role (or multiple new roles) for VACUUM and ANALYZE. > > The primary motivation for this is to continue chipping away at things that > require special privileges or even superuser. VACUUM and ANALYZE typically > require table ownership, database ownership, or superuser. And only > superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these > operations would allow delegating such tasks (e.g., a nightly VACUUM > scheduled with pg_cron) to a role with fewer privileges. Thanks. I'm personally happy with more granular levels of control (as we don't have to give full superuser access to just run a few commands or maintenance operations) for various postgres commands. The only concern is that we might eventually end up with many predefined roles (perhaps one predefined role per command), spreading all around the code base and it might be difficult for the users to digest all of the roles in. It will be great if we can have some sort of rules or methods to define a separate role for a command. > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. I started by trying to introduce > separate pg_vacuum and pg_analyze roles, but that quickly became > complicated because the VACUUM and ANALYZE code is intertwined. To > initiate the discussion, here's the simplest thing I could think of. pg_vacuum_analyze, immediately, makes me think if we need to have a predefined role for CLUSTER command and maybe for other commands as well such as EXECUTE, CALL, ALTER SYSTEM SET, LOAD, COPY and so on. > An alternate approach might be to allow using GRANT to manage these > privileges, as suggested in the previous thread [1]. > > Thoughts? > > [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com > [1] https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de I think GRANT approach [1] is worth considering or at least discussing its pros and cons might give us a better idea as to why we need separate predefined roles. Regards, Bharath Rupireddy.
      • Jump to comment-1
        nathandbossart@gmail.com2022-07-25T16:40:49+00:00
        On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote: > Thanks. I'm personally happy with more granular levels of control (as > we don't have to give full superuser access to just run a few commands > or maintenance operations) for various postgres commands. The only > concern is that we might eventually end up with many predefined roles > (perhaps one predefined role per command), spreading all around the > code base and it might be difficult for the users to digest all of the > roles in. It will be great if we can have some sort of rules or > methods to define a separate role for a command. Yeah, in the future, I could see this growing to a couple dozen predefined roles. Given they are relatively inexpensive and there are already 12 of them, I'm personally not too worried about the list becoming too unwieldy. Another way to help users might be to create additional aggregate predefined roles (like pg_monitor) for common combinations. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
        • Jump to comment-1
          horikyota.ntt@gmail.com2022-07-26T01:47:12+00:00
          At Mon, 25 Jul 2022 09:40:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in > On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote: > > Thanks. I'm personally happy with more granular levels of control (as > > we don't have to give full superuser access to just run a few commands > > or maintenance operations) for various postgres commands. The only > > concern is that we might eventually end up with many predefined roles > > (perhaps one predefined role per command), spreading all around the > > code base and it might be difficult for the users to digest all of the > > roles in. It will be great if we can have some sort of rules or > > methods to define a separate role for a command. > > Yeah, in the future, I could see this growing to a couple dozen predefined > roles. Given they are relatively inexpensive and there are already 12 of > them, I'm personally not too worried about the list becoming too unwieldy. > Another way to help users might be to create additional aggregate > predefined roles (like pg_monitor) for common combinations. I agree to the necessity of that execution control, but I fear a little how many similar roles will come in future (but it doesn't seem so much?). I didn't think so when pg_checkpoint was introdueced, though. That being said, since we're going to control maintenance'ish-command execution via predefined roles so it is fine in that criteria. One arguable point would be whether we will need to put restriction the target relations that Bob can vacuum/analyze. If we need that, the new predeefined role is not sufficient then need a new syntax for that. GRANT EXECUTION COMMAND VACUUM ON TABLE rel1 TO bob. GRANT EXECUTION COMMAND VACUUM ON TABLES OWNED BY alice TO bob. GRANT EXECUTION COMMAND VACUUM ON ALL TABLES OWNED BY alice TO bob. However, one problem of these syntaxes is they cannot do something to future relations. So, considering that aspect, I would finally agree to the proposal here. (In short +1 for what the patch does.) About the patch, it seems fine as the whole except the change in error messages. - (errmsg("skipping \"%s\" --- only superuser can analyze it", + (errmsg("skipping \"%s\" --- only superusers and roles with " + "privileges of pg_vacuum_analyze can analyze it", The message looks a bit too verbose or lengty especially when many relations are rejected. WARNING: skipping "pg_statistic" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can vacuum it WARNING: skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can vacuum it <snip many lines> WARNING: skipping "user_mappings" --- only table or database owner can vacuum it VACUUM Couldn't we simplify the message? For example "skipping \"%s\" --- insufficient priviledges". We could add a detailed (not a DETAIL:) message at the end to cover all of the skipped relations, but it may be too much. By the way the patch splits an error message into several parts but that later makes it harder to search for the message in the tree. *I* would suggest not splitting message strings. # I refrain from suggesing removing parens surrounding errmsg() :p regards. -- Kyotaro Horiguchi NTT Open Source Software Center
          • Jump to comment-1
            robertmhaas@gmail.com2022-07-26T17:37:37+00:00
            On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > One arguable point would be whether we will need to put restriction > the target relations that Bob can vacuum/analyze. Yeah. pg_checkpoint makes sense because you can either CHECKPOINT or you can't. But for a command with a target, you really ought to have a permission on the object, not just a general permission. On the other hand, we do have things like pg_read_all_tables, so we could have pg_vacuum_all_tables too. Still, it seems somewhat appealing to give people fine-grained control over this, rather than just "on" or "off". -- Robert Haas EDB: http://www.enterprisedb.com
            • Jump to comment-1
              david.g.johnston@gmail.com2022-07-26T17:50:32+00:00
              On Tue, Jul 26, 2022 at 10:37 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > One arguable point would be whether we will need to put restriction > > the target relations that Bob can vacuum/analyze. > > But for a command with a target, you really ought to have a > permission on the object, not just a general permission. On the other > hand, we do have things like pg_read_all_tables, so we could have > pg_vacuum_all_tables too. I'm still more likely to create a specific security definer function owned by the relevant table owner to give out ANALYZE (and maybe VACUUM) permission to ETL-performing roles. Still, it seems somewhat appealing to give > people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com David J.
              • Jump to comment-1
                robertmhaas@gmail.com2022-07-26T17:54:38+00:00
                On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston <david.g.johnston@gmail.com> wrote: >> Still, it seems somewhat appealing to give >> people fine-grained control over this, rather than just "on" or "off". > Appealing enough to consume a couple of permission bits? > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com I think we're down to 0 remaining now, so it'd be hard to justify consuming 2 of 0 remaining bits. However, I maintain that the solution to this is either (1) change the aclitem representation to get another 32 bits or (2) invent a different system for less-commonly used permission bits. Checking permissions for SELECT or UPDATE has to be really fast, because most queries will need to do that sort of thing. If we represented VACUUM or ANALYZE in some other way in the catalogs that was more scalable but less efficient, it wouldn't be a big deal (although there's the issue of code duplication to consider). -- Robert Haas EDB: http://www.enterprisedb.com
                • Jump to comment-1
                  nathandbossart@gmail.com2022-07-28T22:26:02+00:00
                  On Tue, Jul 26, 2022 at 01:54:38PM -0400, Robert Haas wrote: > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. AFAICT there are 2 remaining. N_ACL_RIGHTS is only 14. > However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). Perhaps we could add something like a relacl_ext column to affected catalogs with many more than 32 privilege bits. However, if we actually do have 2 bits remaining, we wouldn't need to do that work now unless someone else uses them first. That being said, it's certainly worth thinking about the future of this stuff. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
                • Jump to comment-1
                  horikyota.ntt@gmail.com2022-07-27T05:24:29+00:00
                  At Tue, 26 Jul 2022 13:54:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > <david.g.johnston@gmail.com> wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I guess that we can use the last bit for ACL_SLOW_PATH or something like. Furthermore we could move some existing ACL modeds to that slow path to vacate some fast-ACL bits. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
          • Jump to comment-1
            horikyota.ntt@gmail.com2022-07-26T01:58:14+00:00
            At Tue, 26 Jul 2022 10:47:12 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > WARNING: skipping "pg_statistic" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can vacuum it > WARNING: skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can vacuum it > <snip many lines> > WARNING: skipping "user_mappings" --- only table or database owner can vacuum it By the way, the last error above dissapears by granting pg_vacuum_analyze to the role. Is there a reason the message is left alone? And If I specified the view directly, I would get the following message. postgres=> vacuum information_schema.user_mappings; WARNING: skipping "user_mappings" --- cannot vacuum non-tables or special system tables So, "VACUUM;" does something wrong? Or is it the designed behavior? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
    • Jump to comment-1
      nathandbossart@gmail.com2022-07-25T04:39:31+00:00
      On Fri, Jul 22, 2022 at 01:37:35PM -0700, Nathan Bossart wrote: > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. I started by trying to introduce > separate pg_vacuum and pg_analyze roles, but that quickly became > complicated because the VACUUM and ANALYZE code is intertwined. To > initiate the discussion, here's the simplest thing I could think of. And here's the same patch, but with docs that actually build. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com